Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Batch redraws for mithril timers bench #8

Closed
wants to merge 1 commit into from
Closed

Batch redraws for mithril timers bench #8

wants to merge 1 commit into from

Conversation

simov
Copy link
Contributor

@simov simov commented Aug 25, 2016

Mithril timers bench is now almost twice as fast then Oval 😝

According to Mithril's documentation:

m.redraw is "aggressive": it redraws as many times as it is called (with the caveat that redraws are batched if they occur less than one animation frame apart in time)

Unfortunately that doesn't seems to happen when m.redraw is called multiple times inside each and every timeout. So instead I'm batching the redraws manually.

Mithril doesn't have the concept of redrawing a single component, instead it have only one redraw cycle, and that is the global one. So from Mithril's point of view this fix isn't even a hack, but you can try similar approach with Oval and see how it compares.

@outbounder
Copy link
Member

@simov I guess this diverges from the test case which we have for timers-test in React, Riot & Oval:

  1. represent 2000 div elements with a random value as text
  2. every div element should render by itself on random 10ms framed intervals with its value changed (again at random)

Your PR (as I understand it) is:

  1. represent 2000 div elements with a random value as text
  2. every div element changes its own data value at random 10ms framed interval
  3. the parent containing those 2000 div elements updates all of them at once on 10ms intervals

The bottom line is that I think the PR is an optimization of the implementation which to make a fair relative comparison with other implementations in React, Riot & Oval should be made there too.

So do you think that the above is sound and what do you propose to do further ❓

  • A) implement the optimization to other tests as well

  • B) drop it and mark Mithril's timers-test as N/A :)

    because as you pointed Mithril can't render single components and the test is about

    the ability of the framework/library to asynchronously update 2000 components

    So it sounds rather as test not suitable for Mithril's use cases (compared relatively of course to React, Riot & Oval)

@simov
Copy link
Contributor Author

simov commented Aug 25, 2016

For me the only problem is that Mithril doesn't batch rapid calls to m.redraw as promised. If that was working as advertised you won't even know what Mithril is doing under the hood.

The scenario is pretty much the same:
In React, Riot and Oval you have a method to specify which DOM branch you want to be updated, in Mithril you call global redraw and then Mithril decides what needs to be redrawn. So you have explicit vs implicit.

The only difference between these benchmarks is that the redraw is called outside of the timeout's callback, due to some weird bug in Mithril, the rest is implementation details - it's just how Mithril's redrawing cycle works compared to other frameworks.

To be fair we should have separate global redraw bench for every other framework in that list, because this scenario is much closer to something real. After that we can drop Mithril from this specific bench :)

@outbounder
Copy link
Member

@simov well the each-loop-test via its refresh is just that a 'global' (or top level) redraw. There we have a more precise numbers as well.

@simov
Copy link
Contributor Author

simov commented Aug 25, 2016

I know, that's the next thing on my list.

The point I'm making here is that when you have a swarm of incoming data you generally want to render the entire scene, not each and every element individually.

@simov
Copy link
Contributor Author

simov commented Aug 26, 2016

Closing in favor of #10

@simov simov closed this Aug 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants